Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: master branch is failing precompilation #438

Merged
merged 3 commits into from
Sep 17, 2023
Merged

fix: master branch is failing precompilation #438

merged 3 commits into from
Sep 17, 2023

Conversation

sathvikbhagavan
Copy link
Member

Getting an error:

julia> using Surrogates
[ Info: Precompiling Surrogates [6fc51010-71bc-11e9-0e15-a3fcc6593c49]
ERROR: LoadError: syntax: invalid interpolation syntax: "$$"
Stacktrace:
 [1] top-level scope
   @ ~/oss/Surrogates.jl/src/Radials.jl:37
 [2] include(mod::Module, _path::String)
   @ Base ./Base.jl:457
 [3] include(x::String)
   @ Surrogates ~/oss/Surrogates.jl/src/Surrogates.jl:1
 [4] top-level scope
   @ ~/oss/Surrogates.jl/src/Surrogates.jl:7
 [5] include
   @ ./Base.jl:457 [inlined]
 [6] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2049
 [7] top-level scope
   @ stdin:3

Same error in the CI build on master - https://github.com/SciML/Surrogates.jl/actions/runs/5554601185/job/15046314727

This should fix that

ai-maintainer[bot]
ai-maintainer bot reviewed Aug 28, 2023
View reviewed changes
Copy link

@ai-maintainer ai-maintainer bot left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-Maintainer Review for PR - master branch is failing precompilation

Title and Description 👍

The Title and Description are clear and concise

The title and description of the pull request are clear and concise. They effectively communicate the purpose of the changes, which is to fix the precompilation failure on the master branch. The error message and the link to the CI build where the error occurred provide useful context.

Scope of Changes 👍

The changes are narrowly focused

The changes in this pull request are narrowly focused on resolving a specific issue. The modifications to the Radials.jl file are directly related to fixing the error mentioned in the description. There is no indication that the author is trying to resolve multiple issues simultaneously.

Docstrings 👍

All new functions, classes, or methods have appropriate docstrings

The diff does not show any new functions, classes, or methods being added. Therefore, there are no missing docstrings in the changes made.

Testing 👎

Testing methodology is not described

The description does not provide information about how the author tested the changes. It would be helpful for the author to provide details about the testing methodology to ensure the changes have been thoroughly validated.

Suggested Changes

Please provide information about how you tested the changes. This could include unit tests you ran, manual testing you performed, or any other relevant testing methodology. This will help reviewers understand how you ensured the changes are effective and do not introduce new issues.

Reviewed with AI Maintainer

@sathvikbhagavan sathvikbhagavan changed the title master branch is failing precompilation fix: master branch is failing precompilation Aug 28, 2023
@sathvikbhagavan
Copy link
Member Author

@ChrisRackauckas can we merge this?

@ChrisRackauckas
Copy link
Member

This also fails?

@sathvikbhagavan
Copy link
Member Author

Hmm, its a different error in CI / test (Core 1.6)

✓ Surrogates
✓ CUDA
✓ cuDNN
✓ NNlibCUDA
✓ Flux
133 dependencies successfully precompiled in 157 seconds
   Testing Running tests...
 Resolving package versions...
ERROR: LoadError: Unsatisfiable requirements detected for package Statistics [10745b16]:
Statistics [10745b16] log:
├─possible versions are: 0.0.0 or uninstalled
├─Statistics [10745b16] is fixed to version 0.0.0
└─found to have no compatible versions left with Zygote [e88e6eb3]
 └─Zygote [e88e6eb3] log:
   ├─possible versions are: 0.1.0-0.6.63 or uninstalled
   └─restricted to versions 0.4-0.6 by Surrogates [6fc51010], leaving only versions 0.4.0-0.6.63
     └─Surrogates [6fc51010] log:
       ├─possible versions are: 6.5.1 or uninstalled
       ├─restricted to versions 6 by SurrogatesAbstractGPs [78aa1720], leaving only versions 6.5.1
       │ └─SurrogatesAbstractGPs [78aa1720] log:
       │   ├─possible versions are: 0.1.0 or uninstalled
       │   └─SurrogatesAbstractGPs [78aa1720] is fixed to version 0.1.0
       └─Surrogates [6fc51010] is fixed to version 6.5.1

Surrogates is getting precompiled though. I am not sure why the error is coming though.

@ChrisRackauckas
Copy link
Member

Latest Turing doesn't allow v1.6: is this related?

@sathvikbhagavan
Copy link
Member Author

Surrogates doesn't use Turing.jl afaik

Can you trigger the workflow for the latest julia version - it worked for me locally (although tests were failing - unrelated reason) and it was cancelled in the previous run.

@sathvikbhagavan
Copy link
Member Author

Why does CI / test (Core, 1) job gets cancelled?

@ChrisRackauckas
Copy link
Member

Because v1.6 fails.

@sathvikbhagavan
Copy link
Member Author

Ok, Surrogates does precompile on both but I am not sure why we are getting a version conflict on 1.6. Any ideas to fix this?

@ChrisRackauckas
Copy link
Member

Some package requires >v1.6, something in the GP stack.

@sathvikbhagavan
Copy link
Member Author

One small thing I tried was adding this line in runtests:

Pkg.add(name="Statistics", version=VERSION)

and this worked if I did ]test and did not error out with the package resolution issue in julia 1.6

@sathvikbhagavan
test: add Statistics using Pkg.add
984b6e4 
This is to see if CI will pass or not. This will be removed before
merging
@KristofferC
Copy link

I'll try figure out what is going on here.

@KristofferC
Copy link

The following fixes the issue on 1.6 Pkg:

diff --git a/src/Operations.jl b/src/Operations.jl
index 0644e0ee7..6ac5a48c4 100644
--- a/src/Operations.jl
+++ b/src/Operations.jl
@@ -281,7 +281,7 @@ function collect_project!(ctx::Context, pkg:
:PackageSpec, path::String,
         pkg.version = project.version
     else
         # @warn("project file for $(pkg.name) is missing a `version` entry")
-        pkg.version = VersionNumber(0)
+        pkg.version = VERSION # VersionNumber(0)
     end
     return
 end

Since

Pkg.add(name="Statistics", version=VERSION)

seems to work I would go with that on 1.6 though as a workaround.

@ChrisRackauckas
Copy link
Member

okay thanks let's do that.

ChrisRackauckas
ChrisRackauckas reviewed Sep 14, 2023
View reviewed changes
test/runtests.jl Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit c2fee00 into SciML:master Sep 17, 2023
0 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ai-maintainer ai-maintainer[bot] ai-maintainer[bot] left review comments

@ChrisRackauckas ChrisRackauckas ChrisRackauckas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sathvikbhagavan @ChrisRackauckas @KristofferC